Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename SIRParticleFilter to BootstrapFilter #44

Merged
merged 2 commits into from
Oct 28, 2020
Merged

Rename SIRParticleFilter to BootstrapFilter #44

merged 2 commits into from
Oct 28, 2020

Conversation

zsunberg
Copy link
Member

This fixes #22

@zsunberg zsunberg requested a review from lassepe October 11, 2020 05:12
@zsunberg
Copy link
Member Author

@lassepe do you agree with this name change? @michael-lim do you have any thoughts since you have recently been thinking about particle filters?

@lassepe
Copy link
Member

lassepe commented Oct 11, 2020

To my understanding, "SIR" describes the resampling process while "bootstrap" refers to SIR with a specific proposal distribution (transition prior probability). However, the wording is certainly used in various different ways. I have also found survey papers in which both terms are under interchangeably.

@@ -1,7 +1,7 @@
name = "ParticleFilters"
uuid = "c8b314e2-9260-5cf8-ae76-3be7461ca6d0"
repo = "https://github.com/JuliaPOMDP/ParticleFilters.jl"
version = "0.5.1"
version = "0.5.2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be 0.6.0 now since it breaks existing code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to SemVer this would be okay since all major version 0 releases may be incompatible with eachother. But in Julia there is the convention:

While the semver specification says that all versions with a major version of 0 are incompatible with each other, we have made that choice that a version given as 0.a.b is considered compatible with 0.a.c if a != 0 and c >= b.

Copy link
Member Author

@zsunberg zsunberg Oct 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I actually looked this up (https://semver.org/#how-should-i-handle-deprecating-functionality), and I am still not 100% sure if I understand it correctly it. According to that section, it looks like what you're actually supposed to do is deprecate with a minor version release, and then remove in the next major version (so in this case deprecate in 0.5.2 and remove in 0.6.0).

Before julia 1.5, deprecation warnings were on by default, so there was a big performance penalty for deprecating things, but now they are off by default except in tests. So, I think 0.5.2 is actually correct.

@lassepe
Copy link
Member

lassepe commented Oct 11, 2020

Other than my comment about the version number this looks good to me. I would grep for "SIR" before merging this but I don't have access to my laptop right now.

@zsunberg zsunberg merged commit b4f3ff5 into master Oct 28, 2020
@zsunberg zsunberg deleted the rename-sir branch October 28, 2020 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SIR Particle filter misnamed?
2 participants